Deintegrate CocoaPods for development and use standard Swift Package manager folder structure #77
Conversation
This source filed did not need to be compiled in those targets.
We were no longer importing any dependency via the `Podfile`
This is to be inline with the standard Swift Package setup
This is to be inline with the standard Swift Package setup
Notice all the `import ParselyTracker` are now `import ParselyAnalytics`, which is consistent with what our users would be doing.
I think the reason the tests started to fail with this setup is that the path to the host app for the unit tests changed and that, for some reason still unknown to me, seem to clash with the configure call execution.
Notice this also required updating the test for the generated user agent because the generation logic uses the app name.
| s.test_spec 'Tests' do |test_spec| | ||
| test_spec.source_files = 'Tests' | ||
| test_spec.dependency 'Nimble' | ||
| end |
There was a problem hiding this comment.
This is neat. It will make the unit tests run as part of the pod validation process.
There was a problem hiding this comment.
Maybe we can change the "Build and Test" CI task to run tests using SPM (via xcodebuild test) and CocoaPods (via pod lint)?
| // be in the format | ||
| // | ||
| // xctest/<Xcode version> iOS/<iOS version> (<architecture>) | ||
| expect(RequestBuilder.getUserAgent()).to(match("xctest\\/\\d+\\.\\d+ iOS\\/\\d+\\.\\d+ (.*)")) |
There was a problem hiding this comment.
Gotta love Nimble for offering a RegEx matcher
|
|
||
| func testGlobalPause() { | ||
| // This is call to configure required for the start-stop mechanism to work | ||
| sharedInstance?.configure(siteId: ParselyTestCase.testApikey) |
There was a problem hiding this comment.
What do you think changing this test case to use ParselyTestCase.parselyTestTracker instead of the singleton? You'll still need to call this configure function, but at least there is no potential side effect to other test cases.
There was a problem hiding this comment.
Good point. I used sharedInstance because that's the object the tests run assertions again, but that doesn't mean we cannot rewrite the test to use the test-scoped value.
I got carried away and did a bunch of refactoring around it in #78, which is on top of this PR.
crazytonyli
changed the base branch from
mokagio/merge-resolution-76-reference
to
master
|
@mokagio I took the liberty to change the base branch, because the original one |
crazytonyli
left a comment
crazytonyli
left a comment
There was a problem hiding this comment.
Looks good to me!
This AnalyticsSDKTests seems can be removed.
cwisecarver
left a comment
cwisecarver
left a comment
There was a problem hiding this comment.
These changes all look good to me. Lots of good cleanup work. I am getting sporadic test failures on these two tests. It's not every time and I've never had them both fail but they each seem to fail about every third test run. Seems unlikely that it's related to this PR but I wanted to raise it.
|
⬆️ I reopened this PR to trigger the GitHub Action |
Hat tip @crazytonyli for noticing this. See #77 (review).
Hat tip @crazytonyli for noticing this. See #77 (review).
Because of the Swift Package manager support defined in #68 and fixed in #76 we no longer need to use CocoaPods to integrate the local framework in the demo app. This PR removes CocoaPods and updates the folder structure to the standard SPM setup, with
Sources/andTests/. The demo app now lives inDemo/.Moving around the project had the side effect of changing the test host path for the unit tests and that revealed a unit tests failure in
testGlobalPause(). I had been investigating that since yesterday, but only today found the fix: the test needs to call theconfigure(testId:)method on the shared instance. I haven't dug into the reason for this, but I seems like a useful rabbit hole to go down into.After noticing the test host path change, I decided to update the tests to run without a test host. I find this usually results in a leaner tests execution and removes the chance of state from the demo app leaking in the tests.
As a result of having the local package, I was also able to remove the ParselyTracker development framework. That means that now the demo app and the tests refer to the local Package making the test setup closer to the way our users would integrate the app. (An alternative would be to use CocoaPods to integrate, but with
pod 'ParselyAnalytics', path: '.')I'm leaving this as a draft for now on top of the branchReady to review.mokagio/merge-resolution-76-referenceas I'm waiting for input by @cwisecarver on merge resolution question in #76.